Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: hook pre/post merge include merge source #8703

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Feb 22, 2025

The new field merge_source include source branch/tag/commit as requested.
The current event include source_ref which resolved to the specific commit the merge operation will merge.

Close #8707

The new field `merge_source` include source branch/tag/commit as
requested.
The current event include `source_ref` which resolved to the specific
commit the merge operation will merge.
@nopcoder nopcoder added area/hooks improvements or additions to the hooks subsystem include-changelog PR description should be included in next release changelog labels Feb 22, 2025
@nopcoder nopcoder self-assigned this Feb 22, 2025
Copy link

github-actions bot commented Feb 22, 2025

🎊 PR Preview 4151122 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-8703.surge.sh

🕐 Build time: 0.011s

🤖 By surge-preview

Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

14 passed

Copy link

E2E Test Results - Quickstart

11 passed

@nopcoder nopcoder requested a review from a team February 25, 2025 14:18
@Isan-Rivkin Isan-Rivkin self-requested a review February 27, 2025 08:56
Copy link
Contributor

@Isan-Rivkin Isan-Rivkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks minor stuff about the doc and question about lua

| tag_id[^3] | The ID of the created/deleted tag | string |
| merge_source[^5] | The source branch/tag/ref on merge operation | string |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This renders as 4? (see image with purple arrow)
Screenshot 2025-02-27 at 11 38 12

@@ -73,13 +73,15 @@ Upon execution, a webhook will send a request containing a JSON object with the
| commit_message[^2] | The message for the commit (or merge) that is taking place | string |
| committer[^2] | Name of the committer | string |
| commit_metadata[^2] | The metadata for the commit that is taking place | string |
| commit_id[^2,^4] | The ID of the commit that is being created | string |
| commit_id[^2,^4] | The ID of the commit that is being created | string |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not rendering (see screenshot in comment blueish' arrow)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll fix it

@@ -37,6 +38,7 @@ func marshalEventInformation(actionName, hookID string, record graveler.HookReco
CommitMessage: record.Commit.Message,
Committer: record.Commit.Committer,
CommitMetadata: record.Commit.Metadata,
MergeSource: record.MergeSource.String(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this left out from lua type on purpose? (https://github.com/treeverse/lakeFS/blob/master/pkg/actions/lua.go#L47)

Copy link
Contributor Author

@nopcoder nopcoder Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, It is a bug - will fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hooks improvements or additions to the hooks subsystem include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hooks: Add Source Branch Name to Merge Events
2 participants